-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ add a GitHub Action for shellcheck'ing on every PR and push to master branch + shellcheck'ed script #75
base: master
Are you sure you want to change the base?
Conversation
@hackerb9 , I have rebased and my PR is ready for your assessment… |
44f2b64
to
0828075
Compare
@@ -191,6 +191,8 @@ main() { | |||
readarray -t < <(printf "%s\n" "$@" | sort) | |||
|
|||
# Only show first frame of animated GIFs if filename not specified. | |||
# SC2068 (error): Double quote array expansions to avoid re-splitting elements. | |||
# shellcheck disable=SC2068 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oog. That's ugly. lsix is supposed to be a simple script that people can easily hack upon without all the extra cognitive load of these sort of distractions.
How about just adding the GitHub Action so it can run as an advisory linter but leave all these shellcheck comments out of the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are neccessary for shellcheck
so it won't complain about a specific error or warning in the next line. So for bash-hackers it should not confuse or distract them - but a failing shellcheck
GitHub Action might confuse 🤔
I tried to fix this as mentioned/hinted on https://github.com/koalaman/shellcheck/wiki/SC2068 - but then lsix
failed doing the right thing 😞 So I left shellcheck
intentionally ignore this because it works/does the right thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment that should help other users understand why these shellcheck-lines are neccessary. And I fixed (cd "$arg"; $lsix)
to (cd "$arg" && $lsix)
- Do you @hackerb9 agree to that?
…er branch + shellcheck'ed script
shellcheck
was complaining:This PR fixed this and I tested it:
My GitHub Action's Run can be found here.